-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add topics with zero message counts to the SQLiteStorage::get_metadata(). #1725
Add topics with zero message counts to the SQLiteStorage::get_metadata(). #1725
Conversation
Signed-off-by: Tomoya Fujita <[email protected]>
rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujitatomoya Honestly, I am not a fan of overcomplicated SQL requests.
I found a more simple and elegant solution. See my implementation for Humble #1722
@MichaelOrlov hmmm, i prefer to let the SQL do the job since it is meant to be queried those index data... calling SQL query multiple times and then constructing the data in the program would be more complicated IMO... i guess current SQL query is not complicated at all, seems to me really straight forward. but if you want to take your fix, that is fine too. besides, i think there are slightly different from humble and rolling, right? so if you want to take this PR, i can manage rolling and jazzy on my side. what do you think? |
@fujitatomoya As regards the extra SQL request, we actually have to do this extra SQL request, Yes. the implementation is slightly different on Rolling and Jazzy, this is why I started from Humble. |
@MichaelOrlov i do not have strong opinion on this, so okay to take your fix. do you want me to work on rolling and jazzy? or we should wait #1722 and backport it to rolling and jazzy? |
@fujitatomoya I would appreciate it if you could prepare PR for Rolling, and then we can backport it to Jazzy. |
@MichaelOrlov okay, i will borrow your code and adjust it to rolling, then backport to jazzy! |
…y messages." This reverts commit a842689. Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]>
@MichaelOrlov it now aligns with #1722, CI is passing on my local env. can you take a look? just a slightly structure is different from humble one but complicated. (note, added you as co-author since i borrowed your code.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujitatomoya Thanks, overall looks good.
Only one concern about failure on the Build_and_Test
job. It seems we missed dependencies from the std_msgs
package for test.
Signed-off-by: Tomoya Fujita <[email protected]>
@MichaelOrlov https://build.ros2.org/job/Rpr__rosbag2__ubuntu_noble_amd64/235/ seems unrelated to this change, everything else is green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI from buildfarm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujitatomoya Sorry for the late call - but I found a bug in the implementation.
When creating a TopicMetadata
object, we need to use mapping from the inner_topic_id
to the extern_topic_id
for further comparison with an existing object.
rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomoya Fujita <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@MichaelOrlov can you merge this? |
https://github.com/Mergifyio backport jazzy |
✅ Backports have been created
|
…a(). (#1725) * storage_sqlite3 constructs metadata with topics don't have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * Revert "storage_sqlite3 constructs metadata with topics don't have any messages." This reverts commit a842689. Signed-off-by: Tomoya Fujita <[email protected]> * update metadata list topics that does not have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * add get_metadata_include_topics_with_zero_messages test Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> * add std_msgs to rosbag2_cpp for test build. Signed-off-by: Tomoya Fujita <[email protected]> * call get_or_generate_extern_topic_id when updating metadata. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit ff829e8)
…a(). (#1725) (#1731) * storage_sqlite3 constructs metadata with topics don't have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * Revert "storage_sqlite3 constructs metadata with topics don't have any messages." This reverts commit a842689. Signed-off-by: Tomoya Fujita <[email protected]> * update metadata list topics that does not have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * add get_metadata_include_topics_with_zero_messages test Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> * add std_msgs to rosbag2_cpp for test build. Signed-off-by: Tomoya Fujita <[email protected]> * call get_or_generate_extern_topic_id when updating metadata. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit ff829e8) Co-authored-by: Tomoya Fujita <[email protected]>
…a(). (#1725) (#1731) * storage_sqlite3 constructs metadata with topics don't have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * Revert "storage_sqlite3 constructs metadata with topics don't have any messages." This reverts commit a842689. Signed-off-by: Tomoya Fujita <[email protected]> * update metadata list topics that does not have any messages. Signed-off-by: Tomoya Fujita <[email protected]> * add get_metadata_include_topics_with_zero_messages test Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> * add std_msgs to rosbag2_cpp for test build. Signed-off-by: Tomoya Fujita <[email protected]> * call get_or_generate_extern_topic_id when updating metadata. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit ff829e8) Co-authored-by: Tomoya Fujita <[email protected]> (cherry picked from commit 2a627c9) # Conflicts: # rosbag2_cpp/CMakeLists.txt # rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp
rolling
.I guess we can backport this to
jazzy
.